Skip to content

detect/sip: add sticky buffers to match headers#10839

Closed
glongo wants to merge 10 commits into
OISF:masterfrom
glongo:dev-6374-sip-hdrs-sticky-buffers-v1
Closed

detect/sip: add sticky buffers to match headers#10839
glongo wants to merge 10 commits into
OISF:masterfrom
glongo:dev-6374-sip-hdrs-sticky-buffers-v1

Conversation

@glongo
Copy link
Copy Markdown
Contributor

@glongo glongo commented Apr 14, 2024

Make sure these boxes are signed before submitting your Pull Request -- thank you.

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/6374

Describe changes:
This patchset introduces several sticky buffers to match the following SIP headers:

  • From
  • To
  • Via
  • User-Agent
  • Content-Type
  • Content-Length

SV_BRANCH=OISF/suricata-verify#1764

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 14, 2024

Codecov Report

Attention: Patch coverage is 96.89922% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 82.87%. Comparing base (784ce30) to head (300f5b9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10839      +/-   ##
==========================================
+ Coverage   82.83%   82.87%   +0.04%     
==========================================
  Files         913      921       +8     
  Lines      246847   246975     +128     
==========================================
+ Hits       204474   204679     +205     
+ Misses      42373    42296      -77     
Flag Coverage Δ
fuzzcorpus 64.35% <52.71%> (+0.05%) ⬆️
suricata-verify 62.11% <96.89%> (+0.02%) ⬆️
unittests 62.33% <52.71%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@jlucovsky
Copy link
Copy Markdown
Contributor

I'm looking at RFC3261 and some of the keywords being added like:

  • Content-Type
  • Content-Length
    also have "compact forms (l and c respectively).

I don't know how prevalent the compact form usage is; should your additions handle both forms of the headers iff the header has a compact form?

@glongo
Copy link
Copy Markdown
Contributor Author

glongo commented Apr 15, 2024

I'm looking at RFC3261 and some of the keywords being added like:

* `Content-Type`

* `Content-Length`
  also have "compact forms (`l` and `c` respectively).

I don't know how prevalent the compact form usage is; should your additions handle both forms of the headers iff the header has a compact form?

Definitely, those compact form must be handled.

@catenacyber
Copy link
Copy Markdown
Contributor

Why not a generic sip.request_header keyword ? whose buffer would be name+value like http.request_header (and same for response)

@glongo
Copy link
Copy Markdown
Contributor Author

glongo commented Apr 17, 2024

Why not a generic sip.request_header keyword ? whose buffer would be name+value like http.request_header (and same for response)

I'm not sure I like that to be honest.
@victorjulien, thoughts?

@catenacyber
Copy link
Copy Markdown
Contributor

Why not a generic sip.request_header keyword ? whose buffer would be name+value like http.request_header (and same for response)

I'm not sure I like that to be honest. @victorjulien, thoughts?

Why do not you like it ?
It would allow more signatures expressivity (not restricted to the headers added here), with less lines of code...

@victorjulien
Copy link
Copy Markdown
Member

Why not a generic sip.request_header keyword ? whose buffer would be name+value like http.request_header (and same for response)

I'm not sure I like that to be honest. @victorjulien, thoughts?

Why do not you like it ? It would allow more signatures expressivity (not restricted to the headers added here), with less lines of code...

It's an important keyword for http. In general, the other http keywords like http.header_names, http.start, etc should probably all be recreated here. Rule writers love them :)

@glongo
Copy link
Copy Markdown
Contributor Author

glongo commented Apr 18, 2024

It's an important keyword for http. In general, the other http keywords like http.header_names, http.start, etc should probably all be recreated here. Rule writers love them :)

So, could I add a keyword as @catenacyber suggested in another PR, and leave this one as is once the compact form issue is resolved?

@victorjulien
Copy link
Copy Markdown
Member

It's an important keyword for http. In general, the other http keywords like http.header_names, http.start, etc should probably all be recreated here. Rule writers love them :)

So, could I add a keyword as @catenacyber suggested in another PR, and leave this one as is once the compact form issue is resolved?

I'm ok with doing them in follow up PRs. Btw I suggest a more comprehensive list :)

@glongo
Copy link
Copy Markdown
Contributor Author

glongo commented Apr 18, 2024

It's an important keyword for http. In general, the other http keywords like http.header_names, http.start, etc should probably all be recreated here. Rule writers love them :)

So, could I add a keyword as @catenacyber suggested in another PR, and leave this one as is once the compact form issue is resolved?

I'm ok with doing them in follow up PRs. Btw I suggest a more comprehensive list :)

Sure, it's just a starting point :)

@glongo
Copy link
Copy Markdown
Contributor Author

glongo commented Apr 18, 2024

Replaced with #10907

@glongo glongo closed this Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants